Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add math/base/special/rcbrtf #2185

Merged
merged 11 commits into from
Apr 19, 2024
Merged

feat: add math/base/special/rcbrtf #2185

merged 11 commits into from
Apr 19, 2024

Conversation

gunjjoshi
Copy link
Member

@gunjjoshi gunjjoshi commented Apr 18, 2024

Description

What is the purpose of this pull request?

This pull request:

Related Issues

Does this pull request have any related issues?

None.

Questions

Any questions for reviewers of this pull request?

No.

Other

Any other information relevant to this pull request? This may include screenshots, references, and/or implementation notes.

  • I have used main.c instead of using rcbrtf.c as the filename in src folder, as we are using the same convention for our recent packages.
  • Code Coverage report:
Screenshot 2024-04-18 at 11 21 01

Checklist

Please ensure the following tasks are completed before submitting this pull request.


@stdlib-js/reviewers

@stdlib-bot stdlib-bot added the Math Issue or pull request specific to math functionality. label Apr 18, 2024
Signed-off-by: GUNJ JOSHI <[email protected]>
@kgryte kgryte added the Feature Issue or pull request for adding a new feature. label Apr 18, 2024
@kgryte
Copy link
Member

kgryte commented Apr 18, 2024

Re: using main.c. Thanks for this. Yes, it would be good to standardize on main.c. In most packages, this makes the most sense and ensures consistency. This is the same practice we follow in JavaScript, and we only deviate where warranted.

Signed-off-by: GUNJ JOSHI <[email protected]>
Signed-off-by: GUNJ JOSHI <[email protected]>
Signed-off-by: GUNJ JOSHI <[email protected]>
Copy link
Member

@kgryte kgryte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks, @gunjjoshi!

@gunjjoshi
Copy link
Member Author

I'll reflect the changes in the README and the examples in rcbrt too, in a follow up PR. Thanks @kgryte!

@kgryte
Copy link
Member

kgryte commented Apr 19, 2024

Yeah, for READMEs, we've trended to having the H1 be the alias, rather than a description (e.g., rcbrtf, rather than Reciprocal Cube Root).

And we've tried to steadily refactor examples to eventually make them more amenable to code transformation, such that we can on-the-fly convert syntax from ES5 to ES6+, so we've steadily tried normalizing conventions across examples and reduced the use of unassigned variable declarations, and if needed, juxtaposing them as closely as possible to first usage.

@kgryte
Copy link
Member

kgryte commented Apr 19, 2024

The replacement of round( rand(...) ) is another refactoring stemming from the days before we had implemented discrete-uniform. Opportunities such as this offer chances to "modernize" things a bit now that we have dedicated functionality.

@kgryte kgryte merged commit 8c0fc7e into stdlib-js:develop Apr 19, 2024
8 checks passed
@gunjjoshi
Copy link
Member Author

Great. So for our future implementations too, I'll be following these transformations.

@gunjjoshi gunjjoshi deleted the rcbrtf branch April 19, 2024 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Issue or pull request for adding a new feature. Math Issue or pull request specific to math functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants